Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract common cache data preparation logic to new method #20015

Conversation

salehhashemi1992
Copy link
Contributor

Refactor the multiSet and multiAdd methods by extracting the common logic for preparing cache data into a new private method called prepareCacheData.

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6804fbe) 46.70% compared to head (abcd82f) 48.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20015      +/-   ##
==========================================
+ Coverage   46.70%   48.01%   +1.30%     
==========================================
  Files         445      445              
  Lines       43896    43890       -6     
==========================================
+ Hits        20503    21075     +572     
+ Misses      23393    22815     -578     
Files Coverage Δ
framework/caching/Cache.php 85.24% <100.00%> (+1.65%) ⬆️

... and 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@salehhashemi1992 salehhashemi1992 marked this pull request as ready for review October 19, 2023 14:20
@samdark
Copy link
Member

samdark commented Oct 19, 2023

That's a good refactor but it's a cache and one more function call may matter on many-many cache calls. Are you willing to compare performance?

@salehhashemi1992
Copy link
Contributor Author

salehhashemi1992 commented Oct 19, 2023

I performed a performance test to check the impact of the new method. I ran the test in a loop with 1 million iterations:

        $start = microtime(true);

        for ($i = 0; $i < 1000000; $i++) {
            $cache->flush();

            $cache->multiSet([
                'string_test' => 'string_test',
                'number_test' => 42,
                'array_test' => ['array_test' => 'array_test'],
            ], 0);
        }

        $end = microtime(true);

        echo "Old method elapsed time: " . ($end - $start) . " seconds\n";

I ran the tests three times:

  • ...New method elapsed time: 8.5871288776398 seconds

  • ...New method elapsed time: 8.7551159858704 seconds

  • ...New method elapsed time: 8.8505420684814 seconds

  • ...Old method elapsed time: 8.3703300952911 seconds

  • ...Old method elapsed time: 8.3308370113373 seconds

  • ...Old method elapsed time: 8.4228310585022 seconds

I think the refactored method is slightly slower, but the difference is minimal and likely won't significantly impact real-world performance.

These tests were done using an array cache. In other cache configs, the time taken for network or disk I/O operations would be much greater than in-memory array cache. Therefore, the overhead of an additional function call is likely negligible compared to the time spent on these I/O operations.

@samdark

@samdark
Copy link
Member

samdark commented Oct 20, 2023

Alright. Looks good to me.

@samdark samdark requested review from a team October 20, 2023 13:47
@samdark samdark added this to the 2.0.50 milestone Oct 20, 2023
@salehhashemi1992 salehhashemi1992 force-pushed the feature/extract-prepareCacheData-method- branch from 870316e to abcd82f Compare November 1, 2023 11:42
@salehhashemi1992
Copy link
Contributor Author

@samdark any plan for this PR?

@bizley bizley merged commit a8aa2cf into yiisoft:master Nov 12, 2023
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants